Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdded an example binary that signs a 10KB Ethereum payload and prints the signature. Added Trezor::send_message to send a ProtoMessage without waiting for a response. Changed ethereum_sign_tx and ethereum_sign_eip1559_tx to route each per-chunk ack through handle_interaction(self.call(... )?)? instead of unwrapping with .ok()?. Expanded #[cfg(test)] tests: added with_auto_approve helper, updated the ECDH test to use it, and added test_ethereum_sign_tx_large that signs the 10KB payload against the emulator. Sequence Diagram(s)sequenceDiagram
participant App
participant Client
participant Handler
participant Device
App->>Client: ethereum_sign_tx(path payload)
Client->>Device: send SignTx request
Device-->>Client: EthereumTxRequest (data_length > 0)
loop for each chunk
Client->>Client: prepare TxAck
Client->>Handler: handle_interaction(self.call(ack))
Handler->>Device: transport I/O
Device-->>Handler: chunk response
Handler-->>Client: EthereumTxRequest
end
Device-->>Client: final signature
Client-->>App: return signature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| model | device_test | click_test | persistence_test |
|---|---|---|---|
| T2T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3B1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3W1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
Latest CI run: 25556820684
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/trezor-client/src/client/ethereum.rs`:
- Around line 95-97: The EIP-1559 signing loop in ethereum_sign_eip1559_tx still
uses direct .ok()? which skips mid-stream ButtonRequest handling; update that
loop to mirror the earlier interaction handling by invoking handle_interaction
with the call to self.call(ack, Box::new(|_, m: protos::EthereumTxRequest|
Ok(m)))? and assigning its result to resp (or similar), instead of using .ok()?,
so ButtonRequest/interaction messages are properly processed; ensure you use the
same pattern and error propagation as the existing non-EIP-1559 path
(handle_interaction(...)?).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 721445fa-2444-4985-b2bd-02b63574a219
📒 Files selected for processing (2)
rust/trezor-client/examples/eth_sign_tx_large.rsrust/trezor-client/src/client/ethereum.rs
martykan
force-pushed
the
fix/trezor-client-eth-sign-handle-button-requests
branch
from
3f30807 to
d9ba6b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/trezor-client/src/lib.rs`:
- Around line 271-274: Replace the use of
tracing_subscriber::fmt().with_max_level(tracing::Level::TRACE).init() with
try_init() in the test functions so multiple tests can install the global
subscriber without panicking; update both test_ethereum_sign_tx_large (currently
calling .init()) and test_ecdh_shared_secret (the other test that calls .init())
to call .try_init() and handle the Result (e.g., ignore or unwrap appropriately)
so the tests don't panic when run together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad14e78e-87d5-466a-928c-026454caa0ba
📒 Files selected for processing (2)
rust/trezor-client/src/client/mod.rsrust/trezor-client/src/lib.rs
mmilata
left a comment
mmilata
left a comment
There was a problem hiding this comment.
Would prefer not to use threads unless justified, but for tests I suppose it's OK. Thanks!
|
Squashing before merge. |
romanz
force-pushed
the
fix/trezor-client-eth-sign-handle-button-requests
branch
from
d1c3efd to
07e9e85
Compare
romanz
added
the
no-QA
3 participants
A customer reported an issue when signing using Trezor T with Foundry, which is a CLI tool for EVM development that integrates with Trezor through
trezor-client.These contracts deployment TXs are often quite large (several kilobytes) and the customer found that the process froze while loading.
The root cause was that the TX data streaming was interrupted by a
ButtonRequest, which was not handled bytrezor-clientlogic.This PR adds
handle_interactionwhen streaming the TX data.Can be tested using:
After the fix is merged we should consider making a
trezor-clientrelease